Skip to content

Ensure multiple postgresql::server::recovery resources can be defined #1348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 4, 2022
Merged

Ensure multiple postgresql::server::recovery resources can be defined #1348

merged 2 commits into from
Aug 4, 2022

Conversation

Deroin
Copy link
Contributor

@Deroin Deroin commented Jun 28, 2022

As postgresql::server::recovery is a define it should support multiple instances of itself.
With the concat::fragment having a fixed name this is not possible. Thus I just interpolated the name property into that so that one may create multiple recovery files in different locations, if required.

@Deroin Deroin requested a review from a team as a code owner June 28, 2022 13:13
@CLAassistant
Copy link

CLAassistant commented Jun 28, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

postgresql::server::recovery is a type

that may have no external impact to Forge modules.

This module is declared in 70 of 579 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@Deroin
Copy link
Contributor Author

Deroin commented Jul 13, 2022

FYI I did accept the license agreement, system just doesn't seem to recognize it and the "recheck" link doesn't work.

@ekohl
Copy link
Collaborator

ekohl commented Jul 13, 2022

Can you try to rebase and force push? Perhaps that retriggers it properly. Other than that this change looks good.

@Deroin Deroin force-pushed the allow_multiple_recovery_instances branch from 48c3ad0 to 6995d1d Compare July 13, 2022 12:21
@david22swan
Copy link
Member

@Deroin Look's like you have a few spec test failures:

Failures:
  1) postgresql::server::recovery managing recovery is expected to contain Concat::Fragment[recovery.conf] with content =~ /restore_command = 'restore_command'[\n]+recovery_target_timeline = 'recovery_target_timeline'/
     Failure/Error:
       is_expected.to contain_concat__fragment('recovery.conf')
         .with(content: %r{restore_command = 'restore_command'[\n]+recovery_target_timeline = 'recovery_target_timeline'})
       expected that the catalogue would contain Concat::Fragment[recovery.conf]
     # ./spec/defines/server/recovery_spec.rb:32:in `block (3 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.5.0/bin/rspec:23:in `load'
     # ./vendor/bundle/ruby/2.5.0/bin/rspec:23:in `<top (required)>'
     # ./vendor/bundle/ruby/2.5.0/bin/bundle:23:in `load'
     # ./vendor/bundle/ruby/2.5.0/bin/bundle:23:in `<main>'
  2) postgresql::server::recovery managing recovery with all params is expected to contain Concat::Fragment[recovery.conf] with content =~ /restore_command = 'restore_command'[\n]+archive_cleanup_command = 'archive_cleanup_command'[\n]+recovery_end_command = 'recovery_end_command'[\n]+recovery_target_name = 'recovery_target_name'[\n]+recovery_target_time = 'recovery_target_time'[\n]+recovery_target_xid = 'recovery_target_xid'[\n]+recovery_target_inclusive = true[\n]+recovery_target = 'recovery_target'[\n]+recovery_target_timeline = 'recovery_target_timeline'[\n]+pause_at_recovery_target = true[\n]+standby_mode = on[\n]+primary_conninfo = 'primary_conninfo'[\n]+primary_slot_name = 'primary_slot_name'[\n]+trigger_file = 'trigger_file'[\n]+recovery_min_apply_delay = 0[\n]+/
     Failure/Error:
       is_expected.to contain_concat__fragment('recovery.conf')
         .with(content: %r{restore_command = 'restore_command'[\n]+archive_cleanup_command = 'archive_cleanup_command'[\n]+recovery_end_command = 'recovery_end_command'[\n]+recovery_target_name = 'recovery_target_name'[\n]+recovery_target_time = 'recovery_target_time'[\n]+recovery_target_xid = 'recovery_target_xid'[\n]+recovery_target_inclusive = true[\n]+recovery_target = 'recovery_target'[\n]+recovery_target_timeline = 'recovery_target_timeline'[\n]+pause_at_recovery_target = true[\n]+standby_mode = on[\n]+primary_conninfo = 'primary_conninfo'[\n]+primary_slot_name = 'primary_slot_name'[\n]+trigger_file = 'trigger_file'[\n]+recovery_min_apply_delay = 0[\n]+}) # rubocop:disable Layout/LineLength
       expected that the catalogue would contain Concat::Fragment[recovery.conf]
     # ./spec/defines/server/recovery_spec.rb:103:in `block (3 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.5.0/bin/rspec:23:in `load'
     # ./vendor/bundle/ruby/2.5.0/bin/rspec:23:in `<top (required)>'
     # ./vendor/bundle/ruby/2.5.0/bin/bundle:23:in `load'
     # ./vendor/bundle/ruby/2.5.0/bin/bundle:23:in `<main>'

Look's like some simple errors from the test's not being updated.
Should be an easy fix.

@Deroin Deroin force-pushed the allow_multiple_recovery_instances branch 2 times, most recently from 44d40a6 to 6cc1aea Compare July 26, 2022 13:08
@Deroin Deroin force-pushed the allow_multiple_recovery_instances branch from 6cc1aea to 0944a35 Compare July 26, 2022 13:08
david22swan
david22swan approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry for the wait on the merge for this, had a bit of discussion on whether it changing the recovery files name counted as backwards incompatible or not.
In the end just decided on BugFix.

Anyway, thanks for putting in the work :)

@david22swan david22swan merged commit e53ff08 into puppetlabs:main Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david22swan david22swan david22swan approved these changes

Assignees
No one assigned
Labels
bugfix community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Deroin @CLAassistant @ekohl @david22swan @ia-content